Skip to content

Feat: implement ExchangeRateProvider for Czech National Bank#821

Open
deuveme wants to merge 20 commits into
MewsSystems:masterfrom
deuveme:feat/exchange-rate-provider
Open

Feat: implement ExchangeRateProvider for Czech National Bank#821
deuveme wants to merge 20 commits into
MewsSystems:masterfrom
deuveme:feat/exchange-rate-provider

Conversation

@deuveme
Copy link
Copy Markdown

@deuveme deuveme commented Apr 22, 2026

What

Implementation of ExchangeRateProvider for the Czech National Bank (CNB) using their public daily exchange rate endpoint.

Data source: https://www.cnb.cz/en/financial_markets/foreign_exchange_market/exchange_rate_fixing/daily.txt

Design decisions

The original skeleton was a single flat file. I reorganised it into three layers to keep concerns separate:

  • DomainCurrency, ExchangeRate, CnbDailyTxtParser. No external dependencies.
  • ApplicationIExchangeRateProvider interface. Decouples consumers from the implementation.
  • InfrastructureExchangeRateProvider, HTTP client, configuration. All I/O lives here.

Runtime — Upgraded the project from .NET 6 (original skeleton) to .NET 10. .NET 6 reached end of life in November 2024 and no longer receives security patches. .NET 10 is the current LTS release and also enables modern C# features used throughout the implementation (<Nullable>enable</Nullable>, collection expressions, ArgumentNullException.ThrowIfNull, TimeProvider).

Program — The original skeleton instantiated ExchangeRateProvider directly and printed rates via Console.WriteLine with a hardcoded currency list. Replaced with a proper DI container, structured logging, command-line currency arguments, and exit codes.

Implementation highlights

Parser — handles the pipe-delimited format, normalises amounts greater than 1 (e.g. JPY is quoted per 100 units), and supports both comma and dot as decimal separators since CNB uses comma on the Czech endpoint and dot on the English one.

Caching — Results are cached in memory with a configurable TTL (default 1 hour). Concurrent requests are coalesced via SemaphoreSlim so only one HTTP call is made at a time. If-Modified-Since / HTTP 304 support avoids re-parsing unchanged data. Cache is preserved on errors so a transient failure does not evict valid data.

Resilience — Polly retry pipeline with exponential backoff and jitter on 408, 429, 5xx and network exceptions. All parameters (URL, timeout, retries, TTL) are externalised to appsettings.json and validated at startup.

Contract — Respects all three rules from the original skeleton: only rates published by CNB are returned, no inverse rates are calculated, and unknown currencies are silently ignored.

How to run

cd jobs/Backend/Task
dotnet run                        # defaults to EUR and USD
dotnet run -- EUR USD JPY GBP    # specific currencies

Tests

Unit tests covering: parser edge cases, TTL and cache behaviour, HTTP 304 handling, concurrency (single HTTP request under concurrent callers), error propagation, snapshot preservation on failure, and domain invariants.

cd jobs/Backend
dotnet test Task.Tests/

Open questions for the team

  • Parser strictness — currently a malformed line throws and aborts the entire fetch. An alternative would be to skip invalid lines and log a warning, continuing with the valid ones. Depends on whether partial data is acceptable or not.

  • Cache TTL — defaulting to 1 hour, but CNB publishes once per day (~14:30 CET). The right value depends on how time-sensitive the rates are for the business.

  • Stale cache on CNB outage — currently if the CNB is down and the cache has expired, the error propagates to the caller. An alternative would be to serve the last known snapshot regardless of TTL, accepting stale data over no data. Depends on whether partial availability is preferable to a hard failure for the business.

  • CZK as fixed target currency — all rates are expressed as X/CZK. If the business needs CZK/X, cross rates between non-CZK currencies, or rates against a different base, the current design would need to change.

  • Single daily fixing — CNB publishes one rate per currency per day. If the business requires bid/ask spreads, intraday rates, or historical lookups, a different data source would be needed.

deuveme and others added 20 commits April 22, 2026 10:09
.NET 6 reached EOL in November 2024. Upgrading to .NET 10 LTS
which is supported until November 2028.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ZeroTtl guard is unreachable given [Range(1, int.MaxValue)] on CacheTtlSeconds.
Currency.Code is already uppercased and trimmed by the constructor, so the
ToUpperInvariant()/Trim() calls in NormalizeRequestedCodes were redundant.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The CNB txt feed uses comma decimals (cs-CZ) by default, but the English
endpoint uses dots. TryParseCnbNumber falls back to InvariantCulture when
the token contains a dot but no comma, making the parser endpoint-agnostic.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…y test

ManualResetEventSlim guarantees task1 is holding the refresh semaphore
before task2 starts, eliminating the timing dependency on a fixed sleep.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tion

GetExchangeRates is now a genuine sync method: HttpClient.Send(), SemaphoreSlim.Wait(),
and ReadAsStream()+StreamReader instead of their async counterparts. Removes the
.GetAwaiter().GetResult() anti-pattern and the using-HttpRequestMessage-before-task-completes
bug from SendRequest. IExchangeRateProvider exposes only the sync contract that DotNet.md requires.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- OperationCanceledException instead of TimeoutException in ShouldHandle:
  HttpClient throws TaskCanceledException on timeout, not TimeoutException
- Remove IsNullOrWhiteSpace(currency.Code): Currency constructor already
  guarantees Code is non-null and non-whitespace
- StreamReader with leaveOpen: true so the enclosing using Stream owns
  disposal and the reader does not double-dispose it

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…nforcement

- Enable <Nullable>enable</Nullable> and fix all nullable warnings
- Fix broken string truncation in CnbDailyTxtParser error messages
- Throw ArgumentException on null items in currencies collection (was silently ignored)
- Implement IDisposable on ExchangeRateProvider to dispose SemaphoreSlim
- Add LogDebug for cache hits and log level config for clean output
- Silence HttpClient and Polly info logs, surface Polly warnings

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings April 22, 2026 15:28
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Implements a concrete IExchangeRateProvider backed by the Czech National Bank (CNB) daily TXT endpoint, restructuring the original skeleton into Domain/Application/Infrastructure layers and adding configuration, resilience, caching, and tests.

Changes:

  • Added CNB TXT parsing and domain invariants (Currency, ExchangeRate, CnbDailyTxtParser).
  • Implemented an HTTP-based provider with in-memory caching, conditional GET (If-Modified-Since / 304), and retry resilience via Microsoft.Extensions.Http.Resilience.
  • Reworked the console entrypoint to use DI + options/configuration, and added a full xUnit test project.

Reviewed changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
jobs/Backend/Task/appsettings.json Adds logging + CNB provider configuration (URL/timeout/retries/cache TTL).
jobs/Backend/Task/Program.cs Switches to Generic Host + DI, wiring options, HttpClient resilience, and CLI currency args.
jobs/Backend/Task/Infrastructure/ExchangeRateProvider.cs Implements CNB-backed provider with caching, 304 handling, concurrency coalescing, and parsing integration.
jobs/Backend/Task/Infrastructure/CnbOptionsValidator.cs Adds URL validation for CNB options.
jobs/Backend/Task/Infrastructure/CnbOptions.cs Defines strongly-typed CNB configuration options with data annotations.
jobs/Backend/Task/Infrastructure/CacheSnapshot.cs Introduces a cache snapshot record for rates + timestamps/Last-Modified.
jobs/Backend/Task/ExchangeRateUpdater.sln Updates solution metadata and includes the new test project.
jobs/Backend/Task/ExchangeRateUpdater.csproj Upgrades target framework and adds required hosting/http/options packages + appsettings copy rules.
jobs/Backend/Task/ExchangeRateProvider.cs Removes old placeholder provider implementation.
jobs/Backend/Task/ExchangeRate.cs Removes old domain model location in favor of layered domain.
jobs/Backend/Task/Currency.cs Removes old currency model location in favor of layered domain.
jobs/Backend/Task/Domain/ExchangeRate.cs Adds validated, immutable exchange rate domain model.
jobs/Backend/Task/Domain/Currency.cs Adds currency normalization and basic invariants.
jobs/Backend/Task/Domain/CnbDailyTxtParser.cs Adds CNB daily.txt parser with decimal normalization and strict validation.
jobs/Backend/Task/Application/IExchangeRateProvider.cs Adds application-layer provider contract.
jobs/Backend/Task.Tests/Task.Tests.csproj Adds xUnit test project referencing the main project.
jobs/Backend/Task.Tests/Infrastructure/ExchangeRateProviderTests.cs Adds extensive tests for caching, 304 behavior, concurrency, and error handling.
jobs/Backend/Task.Tests/Infrastructure/CnbOptionsValidatorTests.cs Adds tests for CNB URL option validation.
jobs/Backend/Task.Tests/Domain/DomainInvariantsTests.cs Adds tests for Currency/ExchangeRate invariants.
jobs/Backend/Task.Tests/Domain/CnbDailyTxtParserTests.cs Adds parser correctness + edge case tests.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +51 to +58
HostApplicationBuilder builder = Host.CreateApplicationBuilder(settings);

builder.Services
.AddOptions<CnbOptions>()
.Bind(builder.Configuration.GetSection(CnbOptions.SectionName))
.ValidateDataAnnotations()
.ValidateOnStart();

[Range(0, int.MaxValue)]
public int MaxRetries { get; set; } = 3;

[Range(1, int.MaxValue)]
}

using Stream stream = response.Content.ReadAsStream();
using StreamReader reader = new(stream, leaveOpen: true);
@vgmello-mews
Copy link
Copy Markdown

Code review

Found 2 issues:

  1. Last-Modified is read from response.Content.Headers.LastModified (a content-entity header) instead of the correct response.Headers.LastModified (a response header per RFC 7231). Real CNB servers send Last-Modified as a standard response header, so this always returns null in production — the If-Modified-Since / HTTP 304 optimization is silently disabled against the live endpoint. Tests pass only because the test helper also sets response.Content.Headers.LastModified (mirroring the same mistake). Fix: change to response.Headers.LastModified.

https://github.com/deuveme/developers/blob/401e9c249eb9aebf7d5faa169a255da3193bb40f/jobs/Backend/Task/Infrastructure/ExchangeRateProvider.cs#L108-L112

  1. When FetchSnapshot throws (HTTP error or parse failure), _snapshot is never updated, so the stored snapshot retains its original (now expired) CachedAt timestamp. On every subsequent call, IsFresh returns false and the code immediately retries the live HTTP endpoint — indefinitely, with no cooldown. During a sustained CNB outage every caller hammers the endpoint in a tight loop; the TTL provides no back-pressure protection for the error case. The Polly retry policy in Program.cs mitigates individual request failures, but it does not prevent the provider from re-entering FetchSnapshot on each new consumer call after an expired cache.

https://github.com/deuveme/developers/blob/401e9c249eb9aebf7d5faa169a255da3193bb40f/jobs/Backend/Task/Infrastructure/ExchangeRateProvider.cs#L61-L67

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants